Skip to content

feat(TMC-15843): disabled item in datalist#1990

Merged
VolodymyrKovalM merged 19 commits intomasterfrom
vkoval/feat/TMC-15843/disabled-item-in-datalist
Feb 4, 2019
Merged

feat(TMC-15843): disabled item in datalist#1990
VolodymyrKovalM merged 19 commits intomasterfrom
vkoval/feat/TMC-15843/disabled-item-in-datalist

Conversation

@VolodymyrKovalM
Copy link
Contributor

What is the problem this PR is trying to solve?
https://jira.talendforge.org/browse/TMC-15843
In TMC we need a possibility to make disabled item in Datalist

What is the chosen solution to this problem?

Please check if the PR fulfills these requirements

  • The PR commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features) And non reg done before need review
  • Docs have been added / updated (for bug fixes / features)
  • Related design / discussions / pages (not in jira), if any, are all linked or available in the PR

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

1 similar comment
@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

Copy link
Contributor

@jsomsanith-tlnd jsomsanith-tlnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should pass props.itemProps to react-virtualized, as a function that returns extra props to place on items (<li>) in order to pass aria-disabled on disables items

Look at https://github.com/moroshko/react-autowhatever/blob/master/src/ItemsList.js#L50


return (
<div className={theme.item} title={title}>
<div className={classNames(theme.item, { [theme.disabled]: item.disabled })} title={title}>
Copy link
Contributor

@frassinier frassinier Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typeahead has its own wrapper to display each item (which has the onClick handler) so you can pass disabled if it's a button or an anchor and aria-disabled props.

You can take a look at #1993 for an example

Copy link
Contributor Author

@VolodymyrKovalM VolodymyrKovalM Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks
if it was button, then yes, but the wrapper is < li > element, so disabled on it won't do anything
we will need to pass a class to it

if (this.props.multiSection) {
newValue = this.state.suggestions[sectionIndex].suggestions[itemIndex];
}
if (newValue.disabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to follow that PR I guess moroshko/react-autowhatever#35

Copy link
Contributor Author

@VolodymyrKovalM VolodymyrKovalM Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, also saw that PR, but it was done more than a year ago (
And as there is no any activity on that PR, there is not guarantee that we will get such functionality til release
But we can follow of course

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

Copy link
Contributor

@frassinier frassinier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just experienced some wrong UX with the demo:

  • We should add cursor: not-allowed on disabled state
  • We should skip disabled items while navigating with keyboard

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

}
} else {
const index = this.props.titleMap.findIndex(item => item.name === value);
const index = this.props.titleMap.findIndex(item => item.value === value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful with that. The variable naming is confusing.
You items are objects {name, value}. But we pass the array of names to Typeahead that return us the selected name, which we store as state.value:/

So in state, value is in fact the item name.
This method intends to highlight automatically the selected option when we open the dropdown. By changing the filter, you break that behavior. It works only if name = value, but with custom titleMap, name != value.

We should change the state var name from value to selectedItemName.
But in another PR to be sure it doesn't interfere with your current PR.
In the meantime, please revert this line and the one in the bloc above.

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@frassinier
Copy link
Contributor

I validated even if I consider that disabled items should not be reachable by keyboard like a simple <select> does actually.

@VolodymyrKovalM VolodymyrKovalM merged commit 765809f into master Feb 4, 2019
@VolodymyrKovalM VolodymyrKovalM deleted the vkoval/feat/TMC-15843/disabled-item-in-datalist branch February 4, 2019 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants